Fix/critic block on spec - #467

…he current generation

_MODEL_PATTERN was ^claude-(opus|sonnet|haiku)-\d+-\d+(-...)?$ — BOTH numbers
required. That cannot express claude-opus-5 / claude-sonnet-5, so a correct config
died at startup with "unknown model alias — expected something like
'claude-opus-4-8'", which reads as a typo and sends the operator to edit the wrong
file. Hit for real: after reinstalling from source the loop refused to start on
critic.model=claude-sonnet-5.

A validator that refuses the current generation of the thing it validates is worse
than no validator — it blocks the right value with a confident error.

Now ^claude-(opus|sonnet|haiku|fable)-\d+(-\d+)?(-[a-z0-9.-]+)?$: the minor version
is optional and `fable` is a real family. Accepts claude-opus-5, claude-sonnet-5,
claude-fable-5, claude-haiku-4-5-20251001 and the older claude-opus-4-8 shape;
still rejects claude-5, opus-5, gpt-4, claude-turbo-5 and the empty string — the
rejection half is asserted so the accept half is not vacuous (NEV-CTL-04).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… on Windows

`signal.signal(signal.SIGUSR1, _pause_toggle)` runs unconditionally in
_install_signal_handlers. On Windows `signal.SIGUSR1` does not exist, so boot
raised AttributeError and `forge-loop run` could not start AT ALL. A pause
CONVENIENCE was taking down the entire loop on an entire platform.

Hit for real: after reinstalling from source the runner died on every start with
"AttributeError: module 'signal' has no attribute 'SIGUSR1'". It also explains the
pre-existing tests/test_runner_boot_rotation.py collection error on Windows.

Guarded with hasattr. Nothing is lost there: pause/resume is driven by
`cfg.pause_file`, which `_short_sleep` already polls every tick — the signal
handler only toggles that touchfile, so the file itself remains the portable
interface.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… round

THE GAP. prep_worker_worktree already fetches a fresh base, so a NEW worker starts
current. prep_repair_worktree did not: it fetched the PR branch and never merged
the base in. So every repair round ran against whatever the base looked like when
the branch was first cut.

Measured on a live repo: a PR reached its FIFTH repair round still sitting on a
base from hours earlier, while other PRs merged underneath it. The worker reasons
about, and the critic reviews against, a repo that no longer exists. Git only
warns about TEXTUAL conflicts — two workers independently "fixing" the same thing
in incompatible ways is completely silent.

THE FIX. _sync_base_into_worktree(): fetch origin/<base>, check rev-list
HEAD..origin/<base>, and if the branch is behind, merge the base in. base_branch is
threaded config -> dispatch -> run_repair_worker -> prep_repair_worktree.

- MERGE, not rebase: the branch is already published as a PR, so rebasing needs a
  force-push and would invalidate the review history the critic's round counting
  reads from disk.
- A CONFLICT ABORTS AND REPORTS (repair_base_sync_conflict) rather than leaving a
  half-merged tree. A stale tree is a bad starting point; a conflicted one is
  worse. The worktree is left exactly as it was.
- Already-current is SILENT — that is the common case and does not deserve an
  event.

TESTS (tests/test_repair_base_sync.py) run against real git repos, not mocks: a
stale branch gains a commit that landed after it was cut while keeping its own
work; a genuine same-line conflict emits the conflict event, leaves no MERGE_HEAD,
and preserves the branch's content; and an already-current branch emits nothing —
meaningful only because the first test proves the emitter can fire (NEV-CTL-04).

Suite unchanged: 13 failed / 478 passed before, 13 failed / 481 passed after — the
same pre-existing Windows failures plus the three new tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…whole loop

THE STRUCTURAL PROBLEM. Repair ticks run their workers SYNCHRONOUSLY: dispatch
collects fut.result() inside the ThreadPoolExecutor, so the tick sits inside the
repair until the worker finishes — up to worker_timeout_s (3600s here). While that
happens NO new issue is dispatched, however high its priority.

Measured on a live repo: two PRs consumed an entire day at 17-44 minutes a round.
The backlog sat untouched, and the product's north-star number went unmeasured the
whole time, because the machine was busy re-reviewing the same two diffs.

The earlier fixes each cover part of it and neither closes it:
- block_on_spec parks a PR when the critic RECOGNISES the issue is at fault;
- the base-sync stops repair rounds running against a stale base.
Neither helps when the critic is simply wrong and confident — a sev1 it will
re-emit forever. There was no bound at all.

THE CAP. critic.max_repair_rounds (default 4, 0 disables). blocking_pr_repairs now
reads count_prior_critic_rounds() and, past the cap, stops selecting the PR, emits
repair_round_cap_reached with the counts, and labels the issue loop:needs-review.

The PR is NOT closed and the branch is NOT touched — the work stays intact and a
human resumes it. Lost throughput is recoverable; lost work is not.

TESTS (tests/test_repair_round_cap.py): a PR past the cap is not selected and the
parking is observable; a PR under the cap IS still selected (NEV-CTL-04 — without
that, the first assertion would pass on a selector that returns nothing for any
reason); cap=0 opts out.

Suite unchanged: 19 failed / 632 passed before, 19 failed / 635 passed after.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ost rounds

Reading all six critic rounds on a real PR (a mutation-harness "instrument" PR)
shows block_on_spec caught only half the generator.

THE PATTERN. An instrument PR builds a measurement tool AND reports what it
measured, so its criteria split in two:

  METHOD  — what the DIFF controls: the harness runs, the operators match what
            they claim, the report's headline numbers describe the same run as
            its table. Real defects live here and MUST block. Two of the six
            rounds were exactly this and were correct: a control-removal regex
            that only matched a bare `return;`, and a report printing
            "planted 24 killed 13 survived 11" while its own table disagreed.

  OUTCOME — what the SUBJECT controls: how many survivors exist, how many rows
            resolved, whether a giver offered at level 1.

The blocking criterion was "every survivor is strengthened or deleted". The PR
found 10 survivors, six of them in acquire.ts / farm.ts / npc.ts — modules it does
not touch — fixed 4 and marked 6 as follow-ups. Round 2 blocked: "a category the
issue does not allow". The author cannot bound that work when opening the PR
because the count is unknown until the instrument runs, so the criterion offers
only scope explosion or non-compliance. Neither failure was caused by the diff.

THE TELL, stated so it generalises: a criterion whose cost SCALES WITH WHAT THE
WORK DISCOVERS. Same shape as ">=8 of the 36 rows filled" and "the run reaches
level 5".

THE CORRECTION: demand disposition and honesty, never a disposition COUNT. Every
finding carries a verdict, nothing is silently dropped, and "deferred — lives in a
module this PR does not touch" is legitimate when it names the file and the reason.
Reporting 10 / fixing 4 / deferring 6 with reasons is COMPLETE; reporting 4 and
hiding 6 is a METHOD defect and still blocks.

Added as Q2b in the satisfiability pre-check (so it fires at round 1, not round 5)
and as a new SpecDefect kind `outcome_not_method`. Round 5 of that PR is the proof
this belongs at the front: the critic wrote "a human/PO must edit issue #168 to
resolve the self-contradiction" — it reached for escalation five rounds late and
had no verdict to put it in.

Explicitly NOT permission to slide: an unrun harness, a wrong operator, a
self-contradicting report or a dropped finding all still block.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…a ceiling

reserved_new_work_slots returned min(RESERVED_NEW_WORK_SLOTS, parallel - 1), i.e.
ONE issue whenever any repair was in flight. So raising `parallel` bought nothing:
the extra workers idled while the ready backlog waited.

Measured live: parallel=2 with one repair dispatched exactly one ready issue and
left the second slot empty for the whole tick — while two fully disjoint issues
were queued (a TypeScript SDK one and a Go manager one, no shared paths).

Repairs now keep exactly the slots they are USING (`repairs_pending`) and
everything left over goes to new work:

    free = parallel - repairs_pending
    return max(0, min(max(reserve, free), parallel - 1))

parallel=3/repairs=1 -> 2 (was 1); parallel=4/repairs=1 -> 3 (was 1);
parallel=4/repairs=2 -> 2 (was 1). Unchanged where it mattered before:
parallel=2/repairs=1 -> 1, parallel=1 -> 0 (repairs win the tick), no repairs -> 0,
no candidates -> 0, reserve<=0 -> 0. The #262 anti-starvation guarantee still holds
(its two tests pass untouched).

test_reserved_count_always_within_zero_to_parallel_minus_one asserted the OLD
constant. Its real invariant (0 <= reserved <= parallel-1) is kept; the second
assertion now states the free-slot contract and says why it changed, rather than
being weakened or deleted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…he SDK's bundled claude

run_maintenance shelled out to a bare "claude". The agent SDK ships its OWN claude
binary and the workers use that one, so a machine can run workers perfectly while
having no `claude` on PATH. This machine is exactly that. FileNotFoundError
[WinError 2] escaped run_maintenance and killed the WHOLE runner every
`maintenance_every_n_ticks` ticks (5 by default).

Observed live: the loop died mid `maintenance_start` with the backlog full and
workers healthy, repeatedly, all afternoon — each death looking like an unrelated
"the loop stopped" until the traceback was read.

Same class as the POSIX-only SIGUSR1 handler fixed earlier: an OPTIONAL feature
ending the service. Two guards, because the resolver and the spawn can fail
independently:

- _claude_executable() prefers PATH, then falls back to the binary bundled with
  claude_agent_sdk (the one the workers already use). None => maintenance is
  SKIPPED with the reason in the outcome, not raised.
- FileNotFoundError/OSError around the spawn itself is caught and degraded, for the
  case where the resolver succeeds and the exec still fails (deleted, not
  executable, bad permissions).

Tests assert both degradations AND that the fallback resolver can actually find the
bundled binary — without that third test the first two would pass against a
resolver that always returns None (NEV-CTL-04).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>